[ISSUE #10666][BUG] Persist commitLog truncate marker - #10816

Codecov Report

❌ Patch coverage is 58.33333% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.29%. Comparing base (b5bc1ff) to head (9091321).
⚠️ Report is 24 commits behind head on develop.

@@              Coverage Diff              @@
##             develop   #10816      +/-   ##
=============================================
+ Coverage      48.26%   48.29%   +0.02%     
- Complexity     13437    13514      +77     
=============================================
  Files           1377     1380       +3     
  Lines         100840   101184     +344     
  Branches       13035    13129      +94     
=============================================
+ Hits           48670    48864     +194     
- Misses         46223    46321      +98     
- Partials        5947     5999      +52     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🤖 Automated Review by RockteMQ-AI

Review of the diff for apache/rocketmq#10816.

Overall: The change correctly adds a durable invalid header at the truncation boundary so that recovery stops exactly there instead of rolling forward into stale data. The failure-path handling that keeps ReputMessageService stopped is the right safety choice.

Findings

🟡 Warning — DefaultMessageStore.java:97 / CommitLog.java:17-50

markerOffset = Math.max(offsetToTruncate, this.getMinPhyOffset()) ensures the marker is never written before the log’s minimum offset, but the subsequent truncateDirtyLogicFiles(offsetToTruncate) and commitLog.truncateDirtyFiles(offsetToTruncate) still receive the original smaller value. If either truncation helper does not internally clamp to minPhyOffset in exactly the same way, the marker position and the actual truncation point could diverge. Verify that both helpers use the same clamping semantics.

🟡 Warning — CommitLog.java:25

The bounds check relativeOffset + 2 * Integer.BYTES > mappedFile.getFileSize() is technically correct (8 bytes), but the code then writes a long at relativeOffset. Using Long.BYTES would make the intent clearer and avoid future confusion.

🟡 Warning — CommitLog.java:62-68

Boundary validation accepts only MESSAGE_MAGIC_CODE and MESSAGE_MAGIC_CODE_V2. Confirm that every valid CommitLog entry type (single, batch, delayed messages, etc.) uses one of these two magic codes; otherwise legitimate truncation boundaries could be rejected.

🟢 Suggestion — CommitLog.java:40

Consider asserting phyOffset <= getMaxPhyOffset() before writing the marker. Writing a marker beyond the logical end of the log should not happen in normal flows, but an explicit guard would make the invariant obvious.

🟢 Suggestion — DefaultMessageStoreTest.java

Add a test that truncates exactly at a mapped-file boundary (e.g., at mappedFileSize) and restarts. The current tests truncate inside the first file or at offset 0; a boundary case would validate the interaction between the marker and MappedFileQueue truncation.

🟢 Suggestion — DefaultMessageStore.java:117-121

The catch clause handles RocksDBException | RuntimeException. This is fine, but consider documenting in a code comment why Error is intentionally not caught (fatal JVM errors should not resume message dispatch on a partially truncated store).

Correctness highlights

• CommitLog.java:30-49: hold() / release() and force() are used correctly, ensuring the marker is durable and the mapped file reference is safe.
• CommitLog.java:52-77: Walking the file from 0 to relativeOffset and validating physicOffset against fileFromOffset + currentPosition is a strong integrity check that prevents truncating mid-message.
• DefaultMessageStore.java:102-121: Persisting the marker before truncating consume queues and CommitLog means a crash leaves the store with a consistent recovery stop point, and keeping ReputMessageService stopped on failure prevents dispatching partially truncated data.

No critical correctness issues were found.

_{This is an automated review. Please verify findings before acting on them.}